❮

[PATCH v1] eliminate duplicate code in table.c

  • Jump to comment-1
    zhjwpku@gmail.com2022-07-21T08:26:38+00:00
    There are some duplicate code in table.c, add a static inline function to eliminate the duplicates. -- Regards Junwang Zhao
    • Jump to comment-1
      alvherre@alvh.no-ip.org2022-07-21T12:18:24+00:00
      On 2022-Jul-21, Junwang Zhao wrote: > There are some duplicate code in table.c, add a static inline function > to eliminate the duplicates. Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we should change these error messages to conform to the same message style. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I love the Postgres community. It's all about doing things _properly_. :-)" (David Garamond)
      • Jump to comment-1
        aleksander@timescale.com2022-07-21T12:41:42+00:00
        Hi Alvaro, > Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we > should change these error messages to conform to the same message style. Good point! Done. -- Best regards, Aleksander Alekseev
        • Jump to comment-1
          amit.kapila16@gmail.com2022-07-21T13:47:21+00:00
          On Thu, Jul 21, 2022 at 6:12 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Alvaro, > > > Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we > > should change these error messages to conform to the same message style. > > Good point! Done. > Yeah, that's better. On again thinking about the function name, I wonder if validate_relation_type() suits here as there is no generic object being passed? -- With Regards, Amit Kapila.
          • Jump to comment-1
            aleksander@timescale.com2022-07-21T14:05:30+00:00
            Hi Amit, > Yeah, that's better. On again thinking about the function name, I > wonder if validate_relation_type() suits here as there is no generic > object being passed? Yep, validate_relation_type() sounds better. -- Best regards, Aleksander Alekseev
            • Jump to comment-1
              zhjwpku@gmail.com2022-07-21T15:10:52+00:00
              btw, there are some typos in Patch v5, %s/ralation/relation/g On Thu, Jul 21, 2022 at 10:05 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Amit, > > > Yeah, that's better. On again thinking about the function name, I > > wonder if validate_relation_type() suits here as there is no generic > > object being passed? > > Yep, validate_relation_type() sounds better. > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
              • Jump to comment-1
                aleksander@timescale.com2022-07-21T15:51:57+00:00
                Hi Junwang, > btw, there are some typos in Patch v5, %s/ralation/relation/g D'oh! > yeah, IMHO validate_relation_kind() is better ;) Cool. Here is the corrected patch. Thanks! -- Best regards, Aleksander Alekseev
                • Jump to comment-1
                  zhjwpku@gmail.com2022-07-21T15:58:06+00:00
                  LGTM On Thu, Jul 21, 2022 at 11:52 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Junwang, > > > btw, there are some typos in Patch v5, %s/ralation/relation/g > > D'oh! > > > yeah, IMHO validate_relation_kind() is better ;) > > Cool. Here is the corrected patch. Thanks! > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
                  • Jump to comment-1
                    horikyota.ntt@gmail.com2022-07-22T02:09:22+00:00
                    Hi. + errmsg("cannot operate on relation \"%s\"", Other callers of errdetail_relkind_not_supported() describing operations concretely. In that sense we I think should say "cannot open relation \"%s\"" here. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
                    • Jump to comment-1
                      amit.kapila16@gmail.com2022-07-22T05:14:49+00:00
                      On Fri, Jul 22, 2022 at 7:39 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > + errmsg("cannot operate on relation \"%s\"", > > Other callers of errdetail_relkind_not_supported() describing > operations concretely. In that sense we I think should say "cannot > open relation \"%s\"" here. > Sounds reasonable to me. This will give more precise information to the user. -- With Regards, Amit Kapila.
                      • Jump to comment-1
                        zhjwpku@gmail.com2022-07-22T08:06:50+00:00
                        Here is the patch v7. Thanks! On Fri, Jul 22, 2022 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Jul 22, 2022 at 7:39 AM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > > + errmsg("cannot operate on relation \"%s\"", > > > > Other callers of errdetail_relkind_not_supported() describing > > operations concretely. In that sense we I think should say "cannot > > open relation \"%s\"" here. > > > > Sounds reasonable to me. This will give more precise information to the user. > > -- > With Regards, > Amit Kapila. -- Regards Junwang Zhao
                        • Jump to comment-1
                          amit.kapila16@gmail.com2022-07-22T10:15:52+00:00
                          On Fri, Jul 22, 2022 at 1:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > Here is the patch v7. Thanks! > LGTM. I'll push this sometime early next week unless there are more suggestions/comments. -- With Regards, Amit Kapila.
            • Jump to comment-1
              aleksander@timescale.com2022-07-21T14:21:35+00:00
              Hi Amit, > Yep, validate_relation_type() sounds better. Or maybe validate_relation_kind() after all? -- Best regards, Aleksander Alekseev
              • Jump to comment-1
                zhjwpku@gmail.com2022-07-21T15:05:38+00:00
                yeah, IMHO validate_relation_kind() is better ;) On Thu, Jul 21, 2022 at 10:21 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Amit, > > > Yep, validate_relation_type() sounds better. > > Or maybe validate_relation_kind() after all? > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
    • Jump to comment-1
      amit.kapila16@gmail.com2022-07-21T11:09:35+00:00
      On Thu, Jul 21, 2022 at 1:56 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > There are some duplicate code in table.c, add a static inline function > to eliminate the duplicates. > Can we name function as validate_object_type, or check_object_type? Otherwise, the patch looks fine to me. Let's see if others have something to say. -- With Regards, Amit Kapila.
      • Jump to comment-1
        aleksander@timescale.com2022-07-21T11:39:33+00:00
        Hi hackers, > > There are some duplicate code in table.c, add a static inline function > > to eliminate the duplicates. > > > > Can we name function as validate_object_type, or check_object_type? > > Otherwise, the patch looks fine to me. Let's see if others have > something to say. LGTM -- Best regards, Aleksander Alekseev
        • Jump to comment-1
          amit.kapila16@gmail.com2022-07-21T11:42:20+00:00
          On Thu, Jul 21, 2022 at 5:09 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > > > There are some duplicate code in table.c, add a static inline function > > > to eliminate the duplicates. > > > > > > > Can we name function as validate_object_type, or check_object_type? > > > > Otherwise, the patch looks fine to me. Let's see if others have > > something to say. > > LGTM > @@ -161,10 +121,32 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode, * * Note that it is often sensible to hold a lock beyond relation_close; * in that case, the lock is released automatically at xact end. - * ---------------- + * ---------------- */ void table_close(Relation relation, LOCKMODE lockmode) I don't think this change should be part of this patch. Do you see a reason for doing this? -- With Regards, Amit Kapila.
          • Jump to comment-1
            aleksander@timescale.com2022-07-21T11:49:26+00:00
            Hi Amit, > I don't think this change should be part of this patch. Do you see a > reason for doing this? My bad. I thought this was done by pgindent. -- Best regards, Aleksander Alekseev